fix(geo): skip WHOIS and estimated location on deactivated devices#1340
fix(geo): skip WHOIS and estimated location on deactivated devices#1340tomsebastian10 wants to merge 6 commits intoopenwisp:masterfrom
Conversation
…openwisp#1325 This commit adds regression tests to ensure that the WHOIS handling for deactivated devices works as expected.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds deactivation guards to WHOIS and Estimated Location flows so processing stops immediately for deactivated devices. WHOISService.process_ip_data_and_location and WHOISService.update_whois_info now return early for deactivated devices, avoiding DB updates, skipping scheduling of the Celery fetch_whois_details task and emission of the whois_lookup_skipped signal. trigger_estimated_location_task now returns early when the device is deactivated. Tests were added to verify enqueuing behavior for active vs deactivated devices, and a management command now includes the device _is_deactivated field in its queryset projection. Sequence Diagram(s)sequenceDiagram
participant Device
participant WHOISService
participant Database
participant CeleryTask as "Celery (fetch_whois_details)"
participant SignalBus as "Signal (whois_lookup_skipped)"
Device->>WHOISService: process_ip_data_and_location(ip, location)
WHOISService->>WHOISService: check device._is_deactivated
alt device deactivated
WHOISService-->>Device: return (skip DB, task, signal)
else device active
WHOISService->>Database: update last_ip / location (in transaction)
Database-->>WHOISService: commit hooks scheduled
WHOISService->>SignalBus: maybe emit whois_lookup_skipped
WHOISService->>CeleryTask: schedule fetch_whois_details.delay on commit
end
sequenceDiagram
participant Device
participant WHOISService
participant Database
participant CeleryTask as "Celery (fetch_whois_details)"
Device->>WHOISService: update_whois_info(device, data)
WHOISService->>WHOISService: check device._is_deactivated
alt device deactivated
WHOISService-->>Device: return (skip validation, staleness, scheduling)
else device active
WHOISService->>WHOISService: validate / check staleness
WHOISService->>Database: persist whois info (transaction)
WHOISService->>CeleryTask: schedule fetch_whois_details.delay if needed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tomsebastian10
changed the title
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| Trigger WHOIS lookup based on the conditions of `_need_whois_lookup`. | ||
| Tasks are triggered on commit to ensure redundant data is not created. | ||
| """ | ||
| if self.device.is_deactived(): |
There was a problem hiding this comment.
CRITICAL: Method name typo - is_deactived() should be is_deactivated() (missing 'a'). This will cause an AttributeError at runtime since the method doesn't exist.
| if self.device.is_deactived(): | |
| if self.device.is_deactivated(): |
| when the data is older than | ||
| ``OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS``. | ||
| """ | ||
| if self.device.is_deactived(): |
There was a problem hiding this comment.
CRITICAL: Method name typo - is_deactived() should be is_deactivated() (missing 'a'). This will cause an AttributeError at runtime.
| if self.device.is_deactived(): | |
| if self.device.is_deactivated(): |
| from datetime import timedelta | ||
| from io import StringIO | ||
| from unittest import mock | ||
| from unittest import mock, mocke |
There was a problem hiding this comment.
TYPO: mocke should be removed - this is an unused import typo.
| from unittest import mock, mocke | |
| from unittest import mock |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All previous issues have been resolved. Changes ReviewedFixes for issue #1325 (prevent WHOIS/estimated location on deactivated devices):
Test improvements:
Verification
Reviewed by kimi-k2.5 · 248,985 tokens |
…openwisp#1325 This commit adds regression tests to ensure that the WHOIS handling for deactivated devices works as expected.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_controller/config/whois/tests/tests.py (1)
24-32:⚠️ Potential issue | 🟡 MinorDuplicate imports —
app_settingsandWHOISServiceare each imported twice.Lines 24/30 both import
app_settings, and Lines 26/32 both importWHOISService. Keep one — the existing relative imports on Lines 30 and 32 are consistent with the rest of the file, so the newly-added absolute-path imports on Lines 24 and 26 should be removed.🧹 Proposed fix
-from openwisp_controller.config import settings as app_settings from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped -from openwisp_controller.config.whois.service import WHOISService from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings from ..handlers import connect_whois_handlers from ..service import WHOISService🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 24 - 32, Remove the duplicate absolute imports: delete the top-level imports "from openwisp_controller.config import settings as app_settings" and "from openwisp_controller.config.whois.service import WHOISService", keeping the existing relative imports "from ... import settings as app_settings" and "from ..service import WHOISService" (so only one app_settings and one WHOISService import remain consistent with the file).openwisp_controller/config/whois/service.py (1)
228-248:⚠️ Potential issue | 🟠 MajorMissing test coverage for the
update_whois_infoguard.The PR description and issue
#1325call out both WHOIS code paths, and this method has the same early-return. The new test class only exercisesprocess_ip_data_and_location. Please add a symmetric regression test that seeds a staleWHOISInforecord for the device'slast_ip, marks the device deactivated, callsupdate_whois_info(), and assertsfetch_whois_details.delayis not scheduled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/service.py` around lines 228 - 248, Add a regression test for WHOIS early-return: create a stale WHOISInfo record for the device.last_ip (modified older than OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS), set device.is_deactived() to True (or mark the device deactivated state used by the code), ensure device.last_ip is a valid public IP and is_whois_enabled is set appropriately, then call WHOISService.update_whois_info() and assert that fetch_whois_details.delay was NOT scheduled; locate logic in update_whois_info and WHOISService._get_whois_info_from_db to seed the DB and mock/assert calls to fetch_whois_details.delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@null`:
- Line 1: The PR contains a Git error message instead of code — replace the
erroneous file content with the actual implementation changes and include guard
checks in the two functions mentioned: in process_ip_data_and_location and in
update_whois_info add an early-return guard that checks the device's
active/deactivated state (e.g., device.is_active, device.status, or by loading
device by device_id and checking its active flag) and skip performing WHOIS
lookups when the device is deactivated; ensure these guards run before any
network/WHOIS calls and update tests/fixtures accordingly so the PR actually
contains the modified functions rather than the git error text.
In `@openwisp_controller/config/whois/service.py`:
- Around line 213-214: The guard is calling a non-existent method is_deactived()
which raises AttributeError and prevents WHOIS saves; replace both calls (in
process_ip_data_and_location and in update_whois_info) to use the correct Device
method is_deactivated() so the deactivated-device check actually runs; update
the occurrences of self.device.is_deactived() to self.device.is_deactivated() in
the methods process_ip_data_and_location and update_whois_info.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1199-1217: The active-path test test_process_ip_runs_when_active
is nondeterministic: enable the organization's whois flag explicitly (set
org.config_settings.whois_enabled = True and save) before invoking WHOISService
so the check doesn't rely on global WHOIS_CONFIGURED; also ensure the device
creation/IP change that triggers process_ip_data_and_location happens inside the
patched context (or create/change the device after applying the `@mock.patch` for
fetch_whois_details.delay) so the mock observes the call; for
test_process_ip_skips_when_deactivated either call self.device.deactivate() or
add a brief comment explaining why setting self.device._is_deactivated = True
in-memory is acceptable, and keep assertions against
fetch_whois_details.delay.call_count as-is.
- Line 5: The import line incorrectly includes a non-existent symbol `mocke`,
causing ImportError during test collection; update the import to only import the
valid `mock` symbol (e.g., change `from unittest import mock, mocke` to `from
unittest import mock`) so the test module can load; reference the incorrect
symbol `mocke` and the import statement to locate and correct the issue.
- Around line 1193-1198: TestWHOISDeactivated fails because it calls
self._create_device() but does not inherit CreateWHOISMixin; update the class
declaration to include CreateWHOISMixin alongside TransactionTestCase (mirror
TestWHOISTransaction's bases) so the _create_device helper is available, then
run the tests to verify setUp runs without AttributeError.
---
Outside diff comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 228-248: Add a regression test for WHOIS early-return: create a
stale WHOISInfo record for the device.last_ip (modified older than
OPENWISP_CONTROLLER_WHOIS_REFRESH_THRESHOLD_DAYS), set device.is_deactived() to
True (or mark the device deactivated state used by the code), ensure
device.last_ip is a valid public IP and is_whois_enabled is set appropriately,
then call WHOISService.update_whois_info() and assert that
fetch_whois_details.delay was NOT scheduled; locate logic in update_whois_info
and WHOISService._get_whois_info_from_db to seed the DB and mock/assert calls to
fetch_whois_details.delay.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 24-32: Remove the duplicate absolute imports: delete the top-level
imports "from openwisp_controller.config import settings as app_settings" and
"from openwisp_controller.config.whois.service import WHOISService", keeping the
existing relative imports "from ... import settings as app_settings" and "from
..service import WHOISService" (so only one app_settings and one WHOISService
import remain consistent with the file).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4efda95c-48aa-4af4-a43f-55d36ba1b331
📒 Files selected for processing (3)
nullopenwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-15T14:06:44.800Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:44.800Z
Learning: In openwisp_controller/geo/base/models.py, ensure that in BaseLocation.save(), when the estimated location feature is disabled, the is_estimated field is not overwritten by user input; instead, revert to its initial database value. This preserves the invariant that is_estimated can only change when the feature is enabled. Consider documenting this behavior and refactoring into a guard or helper to reduce future regressions.
Applied to files:
null
📚 Learning: 2026-03-17T09:20:04.547Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:04.547Z
Learning: In openwisp_controller/connection/tasks.py, the update_config Celery task is defined to take a single argument device_id, which is consistently passed as a string (via str(device.pk)) from openwisp_controller/connection/apps.py. Do not flag str(device_id) in task["args"] as unreliable due to typed arguments, because the task arguments are always strings. When reviewing, treat this as a specific, file-scoped guideline: do not raise issues about string conversion or type of the device_id argument for this task; if needed, add a note in the codebase explaining the argument is intentionally a single string ID and that the calling site ensures string conversion.
Applied to files:
null
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (2)
openwisp_controller/config/whois/tests/tests.py (1)
351-352: LGTM on the query-count bump.Increase from 4 → 7 is consistent with the extra
_is_deactivated=Falsefilter now applied across theclear_last_ipcommand path (seeopenwisp_controller/config/whois/tasks.py:116-122).openwisp_controller/config/whois/service.py (1)
208-226: The early return for deactivated devices is intentional design—deactivated devices should not trigger WHOIS processing or signal emission. The pattern is consistent across multiple methods in WHOISService (bothprocess_ip_data_and_locationandupdate_whois_infohave identical early returns), and the existing test confirms this behavior is expected. Signal receivers should not expect notifications from deactivated devices as the intent is to suppress all related background tasks entirely.
| @@ -0,0 +1 @@ | |||
| fatal: no rebase in progress | |||
There was a problem hiding this comment.
Critical: Invalid file content - Git error message instead of code.
This file contains only a Git error message (fatal: no rebase in progress) rather than actual code changes. The PR objectives indicate that guard conditions should be added to process_ip_data_and_location and update_whois_info functions to skip WHOIS lookups for deactivated devices, but no such code is present in the files provided for review.
Please verify the PR contents and ensure the actual code changes are included.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@null` at line 1, The PR contains a Git error message instead of code —
replace the erroneous file content with the actual implementation changes and
include guard checks in the two functions mentioned: in
process_ip_data_and_location and in update_whois_info add an early-return guard
that checks the device's active/deactivated state (e.g., device.is_active,
device.status, or by loading device by device_id and checking its active flag)
and skip performing WHOIS lookups when the device is deactivated; ensure these
guards run before any network/WHOIS calls and update tests/fixtures accordingly
so the PR actually contains the modified functions rather than the git error
text.
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||
| def test_process_ip_skips_when_deactivated(self, mock_task): | ||
| self.device._is_deactivated = True | ||
|
|
||
| service = WHOISService(self.device) | ||
| service.process_ip_data_and_location() | ||
|
|
||
| self.assertEqual(mock_task.call_count, 0) | ||
|
|
||
| @mock.patch.object(app_settings, "WHOIS_CONFIGURED", True) | ||
| @mock.patch("openwisp_controller.config.whois.service.fetch_whois_details.delay") | ||
| def test_process_ip_runs_when_active(self, mock_task): | ||
| self.device._is_deactivated = False | ||
|
|
||
| service = WHOISService(self.device) | ||
| service.process_ip_data_and_location() | ||
|
|
||
| self.assertEqual(mock_task.call_count, 1) |
There was a problem hiding this comment.
Active-path test is not a reliable regression guard.
Two problems with test_process_ip_runs_when_active:
- It only patches
WHOIS_CONFIGURED=Truebut does not enablewhois_enabledon the organization's config settings.is_whois_enabledfalls back to the globalWHOIS_ENABLED, so whetherfetch_whois_details.delayis scheduled depends on a global default unrelated to the deactivation fix. Explicitly enable it (org.config_settings.whois_enabled = True; org.config_settings.save()) to make the test deterministic — see the pattern intest_whois_task_calledat Lines 483-485. - Calling
self.device.save()insetUpwithlast_ip="8.8.8.8"already triggersprocess_ip_data_and_locationvia the post-save flow; by the time the test body runs, the mock patch isn't active yet. Create the device (or change its IP) inside the patched block somock_task.call_count == 1actually reflects the call under test.
Additionally, test_process_ip_skips_when_deactivated sets _is_deactivated = True in-memory without saving; this happens to work because WHOISService holds the same instance, but a brief comment would prevent future confusion (or use self.device.deactivate() for realism).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/whois/tests/tests.py` around lines 1199 - 1217,
The active-path test test_process_ip_runs_when_active is nondeterministic:
enable the organization's whois flag explicitly (set
org.config_settings.whois_enabled = True and save) before invoking WHOISService
so the check doesn't rely on global WHOIS_CONFIGURED; also ensure the device
creation/IP change that triggers process_ip_data_and_location happens inside the
patched context (or create/change the device after applying the `@mock.patch` for
fetch_whois_details.delay) so the mock observes the call; for
test_process_ip_skips_when_deactivated either call self.device.deactivate() or
add a brief comment explaining why setting self.device._is_deactivated = True
in-memory is acceptable, and keep assertions against
fetch_whois_details.delay.call_count as-is.
Flake8 Fails on Unused ImportsHello @tomsebastian10, The CI build failed due to the following issues:
The test suite also encountered an issue, but it appears to be related to the code style failures. Please address the code style issues and push a new commit. |
…an10/openwisp-controller into fix-whois-deactivated
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
24-32: 🛠️ Refactor suggestion | 🟠 MajorDuplicate imports — remove the newly added absolute imports.
Lines 24 and 26 duplicate lines 30 and 32 (the relative imports
from ... import settings as app_settingsandfrom ..service import WHOISServiceresolve to the exact same symbols). The later relative imports shadow the new absolute ones, making lines 24 and 26 redundant.♻️ Proposed fix
-from openwisp_controller.config import settings as app_settings from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped -from openwisp_controller.config.whois.service import WHOISService from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings from ..handlers import connect_whois_handlers from ..service import WHOISServiceAs per coding guidelines: "Flag unused or redundant code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 24 - 32, The test file imports app_settings and WHOISService twice: the absolute imports "from openwisp_controller.config import settings as app_settings" and "from openwisp_controller.config.whois.service import WHOISService" are duplicated and shadowed by the later relative imports "from ... import settings as app_settings" and "from ..service import WHOISService"; remove the earlier absolute import statements so only the relative imports for app_settings and WHOISService remain to eliminate redundancy.
♻️ Duplicate comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
1205-1211: 🧹 Nitpick | 🔵 TrivialPrefer
deactivate()over in-memory flag flip for realism.Setting
self.device._is_deactivated = Truewithout persisting works only becauseWHOISService(self.device)below holds the same in-memory instance. Usingself.device.deactivate()exercises the real deactivation path (save + signal) and would also catch regressions where a caller reloads the device from DB before invoking the service.♻️ Proposed change
- def test_process_ip_skips_when_deactivated(self, mock_task): - self.device._is_deactivated = True - - service = WHOISService(self.device) - service.process_ip_data_and_location() + def test_process_ip_skips_when_deactivated(self, mock_task): + self.device.deactivate() + mock_task.reset_mock() # deactivate() may trigger save-side effects + + service = WHOISService(self.device) + service.process_ip_data_and_location()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 1205 - 1211, The test currently flips the private in-memory flag self.device._is_deactivated; instead call the public deactivation path by invoking self.device.deactivate() (which persists and emits signals) before instantiating WHOISService(self.device) to more realistically exercise deactivation and catch reload/regression issues; update the test method test_process_ip_skips_when_deactivated to use self.device.deactivate() and keep the assertion that mock_task.call_count == 0 after service.process_ip_data_and_location().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/service.py`:
- Around line 213-214: Add the same deactivation guard used in WHOIS: at the
start of EstimatedLocationService.trigger_estimated_location_task (or the method
that invokes it), call self.device.is_deactivated() and immediately return if
True so no Celery task or signals are enqueued; mirror the WHOIS pattern that
prevents _need_whois_lookup and transaction.on_commit from running for
deactivated devices.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1213-1222: The test test_process_ip_runs_when_active currently
only asserts mock_task.call_count; update it to assert that
fetch_whois_details.delay was called with the expected device and IP so the
scheduled task is for the correct device/initial IP. After creating
WHOISService(self.device) and calling service.process_ip_data_and_location(),
replace or augment the count assertion with an assertion on
mock_task.assert_called_once_with(...) (or inspect mock_task.call_args/kwargs)
verifying the device identifier (e.g., self.device.pk or device.id as used by
the service) and the IP "8.8.8.8" are passed to fetch_whois_details.delay so the
call target and payload are validated.
- Around line 351-353: The test triggers an N+1 due to deferred access of the
_is_deactivated field: the clear_last_ip command loads devices with
.only("last_ip", "organization", "key") but saving each device calls
_check_last_ip() → process_ip_data_and_location() → is_deactivated(), which
dereferences _is_deactivated and issues one SELECT per device; update the device
queryset used by the clear_last_ip command (the .only(...) call) to include
"_is_deactivated" so the field is preloaded and avoids the extra queries.
---
Outside diff comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 24-32: The test file imports app_settings and WHOISService twice:
the absolute imports "from openwisp_controller.config import settings as
app_settings" and "from openwisp_controller.config.whois.service import
WHOISService" are duplicated and shadowed by the later relative imports "from
... import settings as app_settings" and "from ..service import WHOISService";
remove the earlier absolute import statements so only the relative imports for
app_settings and WHOISService remain to eliminate redundancy.
---
Duplicate comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1205-1211: The test currently flips the private in-memory flag
self.device._is_deactivated; instead call the public deactivation path by
invoking self.device.deactivate() (which persists and emits signals) before
instantiating WHOISService(self.device) to more realistically exercise
deactivation and catch reload/regression issues; update the test method
test_process_ip_skips_when_deactivated to use self.device.deactivate() and keep
the assertion that mock_task.call_count == 0 after
service.process_ip_data_and_location().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a686e353-0710-4a86-9b42-c70f282c810f
📒 Files selected for processing (2)
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/service.pyopenwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (1)
openwisp_controller/config/whois/service.py (1)
234-235: LGTM on the second guard.Placed before the public-IP/enabled/staleness checks so no redundant work runs, and consistent with the guard in
process_ip_data_and_location.
| if self.device.is_deactivated(): | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the estimated_location service also has a deactivation guard,
# since issue `#1325` requires both WHOIS and Estimated Location to be skipped.
fd -t f 'service\.py$' openwisp_controller/geo/estimated_location --exec cat {} \;
echo "---"
# Look for any is_deactivated / _is_deactivated usage inside estimated_location
rg -nP -C3 '(is_deactivated|_is_deactivated)' openwisp_controller/geo/estimated_locationRepository: openwisp/openwisp-controller
Length of output: 5427
Estimated Location service is missing the deactivation guard—fix is incomplete.
The WHOIS service guard is correct: is_deactivated() is called before _need_whois_lookup and transaction.on_commit, properly suppressing the Celery task and signal for deactivated devices.
However, issue #1325 explicitly requires the same protection for both WHOIS and Estimated Location. The EstimatedLocationService lacks an equivalent guard in trigger_estimated_location_task(). Deactivated devices will still enqueue estimated location tasks, leaving the fix incomplete.
Add an is_deactivated() guard at the entry point of EstimatedLocationService.trigger_estimated_location_task() (or the method that calls it) to match the WHOIS service implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/whois/service.py` around lines 213 - 214, Add the
same deactivation guard used in WHOIS: at the start of
EstimatedLocationService.trigger_estimated_location_task (or the method that
invokes it), call self.device.is_deactivated() and immediately return if True so
no Celery task or signals are enqueued; mirror the WHOIS pattern that prevents
_need_whois_lookup and transaction.on_commit from running for deactivated
devices.
Flake8 and Commit Message FailuresHello @tomsebastian10, There are two issues with your commit:
Here's an example of the correct format: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/whois/tests/tests.py (1)
24-32:⚠️ Potential issue | 🟡 MinorDuplicate imports for
app_settingsandWHOISService.Lines 24 and 26 are equivalent to the existing relative imports on lines 30 and 32 —
...resolves toopenwisp_controller.configand..serviceresolves toopenwisp_controller.config.whois.servicefrom this test module. The second import rebinds the same name, which flake8 will report as F811. Drop the new absolute imports (keep the existing relative ones to stay consistent with the surrounding imports) and keep only the genuinely newwhois_fetched, whois_lookup_skippedimport.🧹 Proposed fix
-from openwisp_controller.config import settings as app_settings from openwisp_controller.config.signals import whois_fetched, whois_lookup_skipped -from openwisp_controller.config.whois.service import WHOISService from openwisp_utils.tests import SeleniumTestMixin, catch_signal from ....tests.utils import TestAdminMixin from ... import settings as app_settings from ..handlers import connect_whois_handlers from ..service import WHOISServiceAs per coding guidelines: "Flag unused or redundant code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 24 - 32, The test file has duplicate imports rebinding app_settings and WHOISService (the absolute imports at the top shadow the existing relative imports from lines that import app_settings and WHOISService); remove the redundant absolute imports that rebind app_settings and WHOISService and keep the existing relative imports (retain the relative imports of app_settings and WHOISService from the current package) while keeping only the new signal imports (whois_fetched, whois_lookup_skipped) from openwisp_controller.config.signals so there is no F811 duplicate-import error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 24-32: The test file has duplicate imports rebinding app_settings
and WHOISService (the absolute imports at the top shadow the existing relative
imports from lines that import app_settings and WHOISService); remove the
redundant absolute imports that rebind app_settings and WHOISService and keep
the existing relative imports (retain the relative imports of app_settings and
WHOISService from the current package) while keeping only the new signal imports
(whois_fetched, whois_lookup_skipped) from openwisp_controller.config.signals so
there is no F811 duplicate-import error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21a6e4db-6832-429e-946d-a61a2386ed64
📒 Files selected for processing (3)
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (3)
openwisp_controller/geo/estimated_location/service.py (1)
65-67: LGTM — defense-in-depth guard for deactivated devices.The early return mirrors the guard added in
WHOISServiceand correctly short-circuitswhois_fetched_handler/whois_lookup_skipped_handlerpaths that eventually calltrigger_estimated_location_task. Even thoughprocess_ip_data_and_locationalready guards at the source, having this guard here prevents regressions if the task is ever triggered from a different code path.openwisp_controller/config/whois/tests/tests.py (1)
1193-1225: LGTM — prior review feedback fully addressed.
- The class now correctly inherits
CreateWHOISMixinandWHOISTransactionMixinand invokessuper().setUp(), so_create_device()is available.setUpexplicitly enableswhois_enabledon the organization'sconfig_settings, making the active-path test deterministic rather than relying on the globalWHOIS_ENABLEDdefault.test_process_ip_runs_when_activenow asserts kwargs viaassert_called_once_with(...), providing a stronger regression signal than a bare call count.One residual note (not a blocker):
test_process_ip_skips_when_deactivatedstill mutates_is_deactivatedin memory — this works becauseWHOISServiceholds the same instance, butself.device.deactivate()would be more faithful to the production flow.openwisp_controller/config/whois/commands.py (1)
42-42: LGTM — correctly prevents the N+1 regression.Including
_is_deactivatedin.only()preloads the field so thatdevice.save()→_check_last_ip()→process_ip_data_and_location()→is_deactivated()doesn't trigger a deferred-field SELECT per device during iteration. This keepstest_last_ip_management_command_queriesat the expected 4 queries regardless of device count.Based on learnings: "In Django projects, when using select_related() to traverse relations ... the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field in the .only() list" — while this specific addition is for a performance reason rather than a traversal conflict, it's consistent with keeping
.only()aligned with all fields accessed downstream of the queryset.
Flake8 F811 Redefinition ErrorsHello @tomsebastian10, There are Error: Error: Fix: |
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
7a02dde to
75273f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/geo/estimated_location/service.py (1)
65-79:⚠️ Potential issue | 🟡 MinorAdd a test for the deactivation guard in
trigger_estimated_location_task.The guard is exercised only indirectly through WHOIS signal handlers; no test in
openwisp_controller/geo/estimated_location/tests/directly verifies thatcurrent_app.send_taskis not called when a device is deactivated. A dedicated test should deactivate a device and assert the task is not enqueued, ensuring that future refactors (e.g., the handler being called from another code path) do not silently bypass this safety check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/geo/estimated_location/service.py` around lines 65 - 79, Write a new unit test that directly exercises EstimatedLocationService.trigger_estimated_location_task to assert it does not enqueue a task when the device is deactivated: instantiate or retrieve a Device, call device.deactivate() (or set the state so device.is_deactivated() returns True), call EstimatedLocationService(device).trigger_estimated_location_task(ip_address), and assert current_app.send_task was not called (use patch/mock on current_app.send_task or task sending mechanism); reference the service method trigger_estimated_location_task, the guard device.is_deactivated, and the call to current_app.send_task (task name whois_estimated_location_task) so the test fails if the guard is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/commands.py`:
- Around line 40-42: The comment should be extended to explain why
"_is_deactivated" is included in the .only(...) call: because the per-device
device.save() within the loop flows into process_ip_data_and_location(), which
calls is_deactivated() and would otherwise trigger a SELECT per device for a
deferred field; update the comment near the .only("last_ip", "organization",
"key", "_is_deactivated") line to note that "_is_deactivated" is required to
avoid reintroducing an N+1 query (see is_deactivated(),
process_ip_data_and_location(), and test_last_ip_management_command_queries for
context).
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1203-1204: The test test_process_ip_skips_when_deactivated mutates
the private flag self.device._is_deactivated instead of exercising the real
deactivation flow; replace that in-memory assignment with a call to
self.device.deactivate() so the device's last_ip/management_ip are cleared and
the model is persisted (this mirrors production behavior when WHOISService may
fetch a fresh instance), or if you intentionally must keep the in-memory toggle,
add a short comment explaining why the real deactivate() cannot be used.
- Around line 1191-1223: Test coverage missing for the deactivation guard in
WHOISService.update_whois_info: add a test that (1) creates/updates a WHOISInfo
for the device's public last_ip with a timestamp older than
WHOIS_REFRESH_THRESHOLD_DAYS, (2) sets self.device._is_deactivated = True, (3)
instantiates WHOISService(self.device) and calls service.update_whois_info(),
and (4) asserts that fetch_whois_details.delay was not called (mocking
"openwisp_controller.config.whois.service.fetch_whois_details.delay"); reference
WHOISService.update_whois_info, WHOISInfo, and WHOIS_REFRESH_THRESHOLD_DAYS to
locate the logic to exercise.
- Around line 1201-1209: The test test_process_ip_skips_when_deactivated is not
asserting the deactivation guard because the device created in setUp has
last_ip=None; set a public last_ip on the device and save it before
instantiating WHOISService so _need_whois_lookup sees a valid IP, then mark the
device deactivated (self.device._is_deactivated = True or use is_deactivated)
and call WHOISService(self.device).process_ip_data_and_location(); this mirrors
test_process_ip_runs_when_active and ensures the only reason the task is not
scheduled is the deactivation guard in
WHOISService.process_ip_data_and_location/_need_whois_lookup.
---
Outside diff comments:
In `@openwisp_controller/geo/estimated_location/service.py`:
- Around line 65-79: Write a new unit test that directly exercises
EstimatedLocationService.trigger_estimated_location_task to assert it does not
enqueue a task when the device is deactivated: instantiate or retrieve a Device,
call device.deactivate() (or set the state so device.is_deactivated() returns
True), call
EstimatedLocationService(device).trigger_estimated_location_task(ip_address),
and assert current_app.send_task was not called (use patch/mock on
current_app.send_task or task sending mechanism); reference the service method
trigger_estimated_location_task, the guard device.is_deactivated, and the call
to current_app.send_task (task name whois_estimated_location_task) so the test
fails if the guard is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 859dd8f1-01f0-4df2-9986-15fb37eec4a3
📒 Files selected for processing (3)
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/tests/tests.py
Test Failure in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
75273f8 to
2eeff23
Compare
| self.device.deactivate() | ||
|
|
||
| ervice = WHOISService(self.device) | ||
| service.update_whois_info() |
There was a problem hiding this comment.
CRITICAL: Variable name typo - ervice should be service. This causes a NameError at runtime since service is undefined.
| service.update_whois_info() | |
| service = WHOISService(self.device) |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_controller/config/whois/tests/tests.py (1)
1242-1243:⚠️ Potential issue | 🔴 CriticalTypo
ervice→NameErrorat runtime; this test cannot pass.Line 1242 defines
ervice(missings) while line 1243 referencesservice.service.update_whois_info()will raiseNameError: name 'service' is not defined, so this test never reaches itsassert_not_called()check.🐛 Proposed fix
- ervice = WHOISService(self.device) + service = WHOISService(self.device) service.update_whois_info()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/tests/tests.py` around lines 1242 - 1243, There's a typo: the variable is created as "ervice = WHOISService(self.device)" but later referenced as "service.update_whois_info()", causing a NameError; rename the declared variable to "service" (or update the later reference to "ervice") so the WHOISService instance assigned to service is the one whose update_whois_info() method is called, ensuring the test proceeds to the subsequent assert_not_called() check (look for WHOISService and update_whois_info in this test).openwisp_controller/config/whois/commands.py (1)
40-42: 🧹 Nitpick | 🔵 TrivialExtend the comment to also justify
_is_deactivated.The existing comment only motivates the
organizationentry. Without a note, a future cleanup could silently drop_is_deactivatedand reintroduce the N+1 on thedevice.save()→process_ip_data_and_location()→is_deactivated()path thattest_last_ip_management_command_queriespins at 4 queries.♻️ Proposed comment tweak
- # include the FK field 'organization' in .only() so the related - # `organization__config_settings` traversal is not deferred + # Include the FK 'organization' so the `organization__config_settings` + # traversal is not deferred, and '_is_deactivated' so the per-device + # save() -> process_ip_data_and_location() -> is_deactivated() path + # does not trigger an N+1 on the deferred field. .only("last_ip", "organization", "key", "_is_deactivated")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/whois/commands.py` around lines 40 - 42, The comment explaining the .only("last_ip", "organization", "key", "_is_deactivated") call is incomplete; extend it to also justify including "_is_deactivated" so future maintainers don't remove it and reintroduce an N+1 during device.save() → process_ip_data_and_location() → is_deactivated(); update the comment to state that "_is_deactivated" is eagerly loaded to avoid extra DB hits observed by test_last_ip_management_command_queries and to preserve the intended query count when traversing organization__config_settings as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1219-1231: Add a one-line clarifying comment in the
test_process_ip_runs_when_active test explaining that because this test class
uses TransactionTestCase, transaction.on_commit callbacks run immediately (there
is no enclosing atomic block) and that saving the device synchronizes
_initial_last_ip with last_ip, which is why the mock_task assertion succeeds;
reference the test name (test_process_ip_runs_when_active),
WHOISService.process_ip_data_and_location, and the _initial_last_ip/on_commit
behavior to make future migration to TestCase (which defers on_commit) obvious.
- Around line 1233-1245: The test test_update_whois_skips_when_deactivated is
invalid because it never exercises the deactivation branch; fix it by creating a
WHOISInfo row for the device IP that is expired (so update_whois_info would
attempt to schedule work), correct the typo "ervice" → "service", then assert
fetch_whois_details.delay is called for the active device and after calling
self.device.deactivate() call WHOISService(self.device).update_whois_info()
again and assert delay is not called; target WHOISService.update_whois_info, the
WHOISInfo test fixture/creation, and the mocked fetch_whois_details.delay in
your changes.
---
Duplicate comments:
In `@openwisp_controller/config/whois/commands.py`:
- Around line 40-42: The comment explaining the .only("last_ip", "organization",
"key", "_is_deactivated") call is incomplete; extend it to also justify
including "_is_deactivated" so future maintainers don't remove it and
reintroduce an N+1 during device.save() → process_ip_data_and_location() →
is_deactivated(); update the comment to state that "_is_deactivated" is eagerly
loaded to avoid extra DB hits observed by
test_last_ip_management_command_queries and to preserve the intended query count
when traversing organization__config_settings as well.
In `@openwisp_controller/config/whois/tests/tests.py`:
- Around line 1242-1243: There's a typo: the variable is created as "ervice =
WHOISService(self.device)" but later referenced as
"service.update_whois_info()", causing a NameError; rename the declared variable
to "service" (or update the later reference to "ervice") so the WHOISService
instance assigned to service is the one whose update_whois_info() method is
called, ensuring the test proceeds to the subsequent assert_not_called() check
(look for WHOISService and update_whois_info in this test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae9ec19c-6928-40c3-bee7-ba3c3a28eab1
📒 Files selected for processing (3)
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.pyopenwisp_controller/geo/estimated_location/service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:30.883Z
Learning: In openwisp/openwisp-controller, the WHOIS service (openwisp_controller/config/whois/) and the EstimatedLocationService (openwisp_controller/geo/estimated_location/service.py) only ever process public IP addresses. Logging these IPs in error/debug messages is acceptable and not a security concern. Do not flag logging of IP addresses in this codebase as a privacy or security issue.
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/config/whois/commands.pyopenwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
📚 Learning: 2026-01-15T14:06:53.460Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/base/models.py:63-93
Timestamp: 2026-01-15T14:06:53.460Z
Learning: In openwisp_controller/geo/base/models.py, the BaseLocation.save() method protects the is_estimated field from manual changes when the estimated location feature is disabled by reverting it to its initial database value using the elif branch. This is intentional behavior to ensure the field can only be modified when the feature is properly configured.
Applied to files:
openwisp_controller/geo/estimated_location/service.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/geo/estimated_location/service.pyopenwisp_controller/config/whois/tests/tests.py
🔇 Additional comments (1)
openwisp_controller/geo/estimated_location/service.py (1)
66-67: LGTM!The deactivation guard at the entry of
trigger_estimated_location_taskcorrectly short-circuits task enqueueing and mirrors the same pattern introduced inopenwisp_controller/config/whois/service.py(process_ip_data_and_locationandupdate_whois_info). Placing the check before thetry/except send_taskblock is the right spot — callers inhandlers.py(whois_fetched_handlerandwhois_lookup_skipped_handler) don't need to duplicate the check, and no Celery task is scheduled for deactivated devices.
Flake8 Errors in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
2eeff23 to
eadb209
Compare
Test Failures in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
eadb209 to
8633a01
Compare
Flake8 F821 Error in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
8633a01 to
6e6f41e
Compare
Test Failure in
|
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
6e6f41e to
2504f10
Compare
Test Failure in
|
…isp#1325 - add guard in WHOISService to skip processing for deactivated devices - add guard in EstimatedLocationService to prevent task scheduling - fix N+1 query by including _is_deactivated in clear_last_ip_command queryset - add regression tests ensuring tasks are skipped when device is deactivated - ensure deterministic tests by enabling whois on organization settings Fixes openwisp#1325
tomsebastian10
force-pushed
the
fix-whois-deactivated
branch
from
2504f10 to
d235322
Compare
1 participant
Checklist
Reference to Existing Issue
Closes #1325 .
Description of Changes
This PR fixes an issue where WHOIS and Estimated Location lookups were still being triggered for deactivated devices.
Previously, the logic responsible for scheduling WHOIS lookup tasks (process_ip_data_and_location and update_whois_info) did not check whether the device was active. As a result, background tasks continued to run even when a device was deactivated, causing WHOIS and location data to be repopulated unintentionally.
This change introduces a guard condition to skip these operations when the device is inactive:
Added an early return in process_ip_data_and_location
Added an early return in update_whois_info
This ensures that:
No WHOIS lookup tasks are scheduled for deactivated devices
No unnecessary background processing occurs
Device state is respected consistently